fix(rename): handle complex rename/move scenario

ensure we do not leak records and properly update them in client
database

Signed-off-by: Matthieu Gallien <matthieu.gallien@nextcloud.com>

diff --git a/src/libsync/propagatorjobs.cpp b/src/libsync/propagatorjobs.cpp
index 002480c82705d1f124a55ea9e9d7656d78c75280..59b7ec9c98dfacfe8b6d7ae11cd12a6be0a6f284 100644 (file)
--- a/src/libsync/propagatorjobs.cpp
+++ b/src/libsync/propagatorjobs.cpp
@@ -302,8 +302,9 @@ void PropagateLocalRename::start()
     const auto previousNameInDb = propagator()->adjustRenamedPath(_item->_file);
     const auto existingFile = propagator()->fullLocalPath(previousNameInDb);
     const auto targetFile = propagator()->fullLocalPath(_item->_renameTarget);
+    const auto originalFile = propagator()->fullLocalPath(_item->_originalFile);
 
-    const auto fileAlreadyMoved = !FileSystem::fileExists(propagator()->fullLocalPath(_item->_originalFile)) && FileSystem::fileExists(existingFile);
+    const auto fileAlreadyMoved = (!FileSystem::fileExists(originalFile) || !FileSystem::fileExists(existingFile))&& FileSystem::fileExists(targetFile);
     auto pinState = OCC::PinState::Unspecified;
     if (!fileAlreadyMoved) {
         auto pinStateResult = vfs->pinState(propagator()->adjustRenamedPath(_item->_file));
@@ -315,6 +316,8 @@ void PropagateLocalRename::start()
     // if the file is a file underneath a moved dir, the _item->file is equal
     // to _item->renameTarget and the file is not moved as a result.
     qCDebug(lcPropagateLocalRename) << _item->_file << _item->_renameTarget << _item->_originalFile << previousNameInDb << (fileAlreadyMoved ? "original file has already moved" : "original file is still there");
+    qCDebug(lcPropagateLocalRename()) << (FileSystem::fileExists(originalFile) ? "original file exists" : "orignal file is missing") << originalFile << _item->_originalFile;
+    qCDebug(lcPropagateLocalRename()) << (FileSystem::fileExists(existingFile) ? "existing file exists" : "existing file is missing") << existingFile << previousNameInDb;
     Q_ASSERT(FileSystem::fileExists(propagator()->fullLocalPath(_item->_originalFile)) || FileSystem::fileExists(existingFile));
     if (_item->_file != _item->_renameTarget) {
         propagator()->reportProgress(*_item, 0);
@@ -444,7 +447,7 @@ void PropagateLocalRename::start()
 
     if (fileAlreadyMoved && !deleteOldDbRecord(previousNameInDb)) {
         return;
-    } else if (!deleteOldDbRecord(_item->_originalFile)) {
+    } else if (!deleteOldDbRecord(previousNameInDb)) {
         qCWarning(lcPropagateLocalRename) << "Could not delete file from local DB" << _item->_originalFile;
         return;
     }
@@ -470,25 +473,28 @@ void PropagateLocalRename::start()
             done(SyncFileItem::SoftError, tr("The file %1 is currently in use").arg(newItem._file), ErrorCategory::GenericError);
             return;
         }
-    } else {
-        const auto dbQueryResult = propagator()->_journal->getFilesBelowPath(oldFile.toUtf8(), [oldFile, this] (const SyncJournalFileRecord &record) -> void {
-            const auto oldFileName = record._path;
-            const auto oldFileNameString = QString::fromUtf8(oldFileName);
+    } else if (!fileAlreadyMoved) {
+        qCDebug(lcPropagateLocalRename) << "propagate child items after move from" << existingFile << "to" << targetFile;
+        const auto dbQueryResult = propagator()->_journal->getFilesBelowPath(previousNameInDb.toUtf8(), [previousNameInDb, this] (const SyncJournalFileRecord &record) -> void {
+            const auto oldFileNameString = propagator()->adjustRenamedPath(QString::fromUtf8(record._path));
             auto newFileNameString = oldFileNameString;
-            newFileNameString.replace(0, oldFile.length(), _item->_renameTarget);
+            newFileNameString.replace(0, previousNameInDb.length(), _item->_renameTarget);
+
+            qCDebug(lcPropagateLocalRename) << "child rename from" << oldFileNameString << "to" << newFileNameString;
 
             if (oldFileNameString == newFileNameString) {
+                Q_ASSERT(false);
                 return;
             }
 
             SyncJournalFileRecord oldRecord;
-            if (!propagator()->_journal->getFileRecord(oldFileName, &oldRecord)) {
-                qCWarning(lcPropagateLocalRename) << "Could not get file from local DB" << oldFileName;
+            if (!propagator()->_journal->getFileRecord(oldFileNameString, &oldRecord)) {
+                qCWarning(lcPropagateLocalRename) << "Could not get file from local DB" << oldFileNameString;
                 done(SyncFileItem::NormalError, tr("Could not get file %1 from local DB").arg(oldFileNameString), OCC::ErrorCategory::GenericError);
                 return;
             }
-            if (!propagator()->_journal->deleteFileRecord(oldFileName)) {
-                qCWarning(lcPropagateLocalRename) << "could not delete file from local DB" << oldFileName;
+            if (!propagator()->_journal->deleteFileRecord(oldFileNameString)) {
+                qCWarning(lcPropagateLocalRename) << "could not delete file from local DB" << oldFileNameString;
                 done(SyncFileItem::NormalError, tr("Could not delete file record %1 from local DB").arg(oldFileNameString), OCC::ErrorCategory::GenericError);
                 return;
             }

diff --git a/test/testsyncmove.cpp b/test/testsyncmove.cpp
index 4f775c7fc9fd611290fbc384172f81765148874c..246d34882f7877a624a747bba9b5f476a5109eb8 100644 (file)
--- a/test/testsyncmove.cpp
+++ b/test/testsyncmove.cpp
@@ -1317,6 +1317,61 @@ private slots:
         QCOMPARE(counter.nMKCOL, 0);
         QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
     }
+
+    void testRenameComplexScenarioNoRecordLeak()
+    {
+        FakeFolder fakeFolder{FileInfo{}};
+
+        QCOMPARE(fakeFolder.currentLocalState(), fakeFolder.currentRemoteState());
+
+        fakeFolder.remoteModifier().mkdir("0");
+        fakeFolder.remoteModifier().mkdir("0/00 without file");
+        fakeFolder.remoteModifier().mkdir("0/00 without file/project");
+        fakeFolder.remoteModifier().mkdir("0/00 without file/project/a");
+        fakeFolder.remoteModifier().mkdir("0/00 without file/project/a/a with file");
+        fakeFolder.remoteModifier().mkdir("0/00 without file/project/a/a without file");
+        fakeFolder.remoteModifier().mkdir("0/00 without file/project/a/aa with file");
+        fakeFolder.remoteModifier().mkdir("0/00 without file/project/00 with file");
+        fakeFolder.remoteModifier().mkdir("0/00 without file/project/new without file");
+        fakeFolder.remoteModifier().insert("0/00 without file/project/00 with file/test.md");
+        fakeFolder.remoteModifier().insert("0/00 without file/project/a/a with file/test.md");
+        fakeFolder.remoteModifier().insert("0/00 without file/project/a/aa with file/test.md");
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        auto itemsCounter = 0;
+        auto dbResult = fakeFolder.syncJournal().getFilesBelowPath("", [&itemsCounter] (const SyncJournalFileRecord&) -> void { ++itemsCounter; });
+
+        QVERIFY(dbResult);
+        QCOMPARE(itemsCounter, 12);
+
+        fakeFolder.remoteModifier().rename("0/00 without file/project", "project tests");
+        fakeFolder.remoteModifier().rename("project tests/a", "project tests/a empty");
+        fakeFolder.remoteModifier().rename("project tests/a empty/a with file", "project tests/a with file");
+        fakeFolder.remoteModifier().rename("project tests/a empty/a without file", "project tests/a without file");
+        fakeFolder.remoteModifier().rename("project tests/a empty/aa with file", "project tests/aa with file");
+        fakeFolder.remoteModifier().rename("project tests/new without file", "project tests/new without file");
+        fakeFolder.remoteModifier().rename("0/00 without file", "project tests/00 without file");
+        fakeFolder.remoteModifier().rename("0", "project tests/z 0 empty");
+
+        connect(&fakeFolder.syncEngine(), &OCC::SyncEngine::itemCompleted, this, [&fakeFolder] () {
+            auto itemsCounter = 0;
+            auto dbResult = fakeFolder.syncJournal().getFilesBelowPath("", [&itemsCounter] (const SyncJournalFileRecord&) -> void { ++itemsCounter; });
+
+            QVERIFY(dbResult);
+            if (itemsCounter > 12) {
+                QVERIFY(itemsCounter <= 12);
+            }
+        });
+
+        QVERIFY(fakeFolder.syncOnce());
+
+        itemsCounter = 0;
+        dbResult = fakeFolder.syncJournal().getFilesBelowPath("", [&itemsCounter] (const SyncJournalFileRecord&) -> void { ++itemsCounter; });
+
+        QVERIFY(dbResult);
+        QCOMPARE(itemsCounter, 12);
+    }
 };
 
 QTEST_GUILESS_MAIN(TestSyncMove)